Skip to content

firestore: misc.ts: further improved performance of UTF-8 string comparison logic #9143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 7, 2025
Merged

firestore: misc.ts: further improved performance of UTF-8 string comparison logic #9143

merged 1 commit into from
Jul 7, 2025

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jul 3, 2025 •
edited
Loading

Re-implemented Firestore's compareUtf8Strings() function based on the greatly-improved algorithm from firebase/firebase-android-sdk#7098.

This algorithm has gone through a lot of churn recently due to performance issues with the naive implementation. This is (hopefully) the last PR to touch this function due to performance issues. The PR's leading up to this one are:

  1. FIX: sort strings in UTF-8 encoded byte order #8691 (slow, naive implementation)
  2. Revert the UTF-8 encoding in string sorting #8782 (revert the slow, naive impl)
  3. Fix: sort strings in UTF-8 encoded byte order with lazy encoding #8787 (faster, but complex impl)

This PR was ported to the nodejs-firestore repo in googleapis/nodejs-firestore#2380

@dconeybe dconeybe self-assigned this Jul 3, 2025
@dconeybe dconeybe requested review from a team as code owners July 3, 2025 19:41
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2025

🦋 Changeset detected

Latest commit: da7185b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/firestore Patch
firebase Patch
@firebase/firestore-compat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dconeybe dconeybe mentioned this pull request Jul 3, 2025
Merged
@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    browser395 kB395 kB-356 B (-0.1%)
    main616 kB615 kB-555 B (-0.1%)
    module395 kB395 kB-356 B (-0.1%)
    react-native395 kB395 kB-357 B (-0.1%)

  • @firebase/firestore-lite

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    browser117 kB116 kB-347 B (-0.3%)
    main160 kB159 kB-608 B (-0.4%)
    module117 kB116 kB-347 B (-0.3%)
    react-native117 kB116 kB-349 B (-0.3%)

  • bundle

    15 size changes

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    firestore (CSI Auto Indexing Disable and Delete)291 kB290 kB-238 B (-0.1%)
    firestore (CSI Auto Indexing Enable)291 kB290 kB-238 B (-0.1%)
    firestore (Persistence)322 kB322 kB-201 B (-0.1%)
    firestore (Query Cursors)260 kB260 kB-201 B (-0.1%)
    firestore (Query)258 kB258 kB-201 B (-0.1%)
    firestore (Read data once)248 kB247 kB-201 B (-0.1%)
    firestore (Read Write w Persistence)342 kB342 kB-201 B (-0.1%)
    firestore (Realtime updates)248 kB248 kB-201 B (-0.1%)
    firestore (Transaction)227 kB227 kB-238 B (-0.1%)
    firestore (Write data)228 kB228 kB-238 B (-0.1%)
    firestore-lite (Query Cursors)111 kB111 kB-212 B (-0.2%)
    firestore-lite (Query)107 kB107 kB-212 B (-0.2%)
    firestore-lite (Read data once)82.9 kB82.7 kB-212 B (-0.3%)
    firestore-lite (Transaction)108 kB108 kB-212 B (-0.2%)
    firestore-lite (Write data)92.4 kB92.1 kB-212 B (-0.2%)

  • firebase

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    firebase-compat.js807 kB807 kB-174 B (-0.0%)
    firebase-firestore-compat.js351 kB351 kB-174 B (-0.0%)
    firebase-firestore-lite.js140 kB139 kB-351 B (-0.3%)
    firebase-firestore.js459 kB458 kB-366 B (-0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/EyVnYwJDSz.html

@google-oss-bot
Copy link
Contributor

Size Analysis Report 1

This report is too large (864,920 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/WMaZTjQT7E.html

wu-hui
wu-hui approved these changes Jul 4, 2025
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix.

@dconeybe dconeybe changed the title firestore: misc.ts: re-implemented compareUtf8Strings() to be even more simple and efficient firestore: misc.ts: further improved performance of UTF-8 string comparison logic Jul 7, 2025
@dconeybe dconeybe merged commit a029ce3 into main Jul 7, 2025
67 of 69 checks passed
@dconeybe dconeybe deleted the dconeybe/Utf8StringComparePerformanceFix branch July 7, 2025 15:28
dconeybe added a commit to googleapis/nodejs-firestore that referenced this pull request Jul 7, 2025
@dconeybe
fix: Improve performance of the UTF-8 string comparison logic.
5667e5c 
The semantics of this logic were originally fixed by #2275, but this fix
caused a material performance degradation, which was then improved by #2299
The performance was, however, still suboptimal, and this PR further improves the
speed back to close to its original speed and, serendipitously, simplifies the
algorithm too.

This commit is a port of firebase/firebase-js-sdk#9143
This was referenced Jul 7, 2025
Merged
Open
@google-oss-bot google-oss-bot mentioned this pull request Jul 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-hui wu-hui wu-hui approved these changes

Assignees

@dconeybe dconeybe

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dconeybe @google-oss-bot @wu-hui